fix(resolution): Fix resolution of querystringed imports in ESM files#322
Open
lobsterkatie wants to merge 11 commits into
Open
fix(resolution): Fix resolution of querystringed imports in ESM files#322lobsterkatie wants to merge 11 commits into
lobsterkatie wants to merge 11 commits into
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, there are two problems with the way
@vercel/nfthandles ESM imports with querystrings (import * as something from './someFile?someQuerystring';):?as it would any other character. As a result, it fails to resolve such imports to the non-querystringed files to which they correspond. (In the above example, it looks for a file namedsomeFile?someQuerystring.jsand, as one would expect, doesn't find it.) While this is the correct behavior forrequirecalls in CJS files, it doesn't match the way that Node handles ESM imports (which is to strip the querystring off before attempting to resolve the file).resolvemethod is provided, and that custom method does strip querystrings from ESM imports, the querystrings are then not put back on before the imported module is processed. This matters, for example, in cases wherereadFileis also overridden, with a method whose output is affected by the querystring's value. (Webpack is such a system, so one place this shows up is in Next.js'sNextTraceEntryPointsPlugin.)This fixes both of those problems by selectively stripping and re-adding the querystring in ESM imports at various points in the
nodeFileTracelifecycle. More specifically, inresolveDependecyand inemitDependencyand its helpers, we strip the querystring (and/or don't add it back), with the exception of:readFile, we keep the querystring, since some implementations ofreadFile, including the one in the nft webpack plugin, read different modules out of memory with different querystrings).resolve, we add the querystring back in, since this is also something which can be supplied by the user, and and the user-supplied version might care about query strings.emitDependency, we add the querystring back in, since we need it there to be able to start the whole cycle over.Notes:
cjsResolveto the recursive call ofemitDependency. Rather than changeemitDependency's signature, I opted to makeemitDependencya wrapper for an inner function (analyzeAndEmitDependency) with the updated signature. Recursive calls now go to the inner function.@sentry/nextjsteam are interested in this because we use a webpack loader to insert in the dependency tree a proxy module for each page, which allows us to automatically wrap users' exported functions (getServerSidePropsand friends, API route handlers, etc). To do this, we replace the code from/path/to/pages/somePage.jswith our proxy code, which includes an import from'./somePage?__sentry_wrapped__'. When webpack then crawls our proxy module looking for dependencies, the querystring tricks it into thinking it hasn't yet dealt with that page, so it forwards it to our loader a second time rather than skipping it. When our loader sees the querystring, it knows the code replacement has already been done, and returns the original page code untouched, thereby preserving the continuity of the dependency tree. This fix ensures that@vercel/nftsuccessfully traces the modified tree.cjs-querystring) required a little gymnastics in order to not break CI on Windows. It turns out that skipping the test on Windows wasn't enough - the mere fact that a file exists with a?in its name is enough to cause Windows not to even be able to check out the commit. Therefore, the file is stored without any punctuation in its name, and before the tests run on non-Windows platforms, it is copied to a version which does have the punctuation, and which is git-ignored to enforce it not ending up in the repo in the future by accident. The dummy, no-punctuation version is stored in a folder in order to differentiate its path from that of the punctuated version by more than just the punctuation. That way, if the punctuated version is found, it's clear it's not just because the punctuation is somehow being ignored.Fixes getsentry/sentry-javascript#5998.
Fixes #319.